-
Notifications
You must be signed in to change notification settings - Fork 78
Use boolean allocation options #1400
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
We remove OnAllocationFail and add three boolean fields to AllocationOptions.
59ff447
to
fa47af7
Compare
After this PR, the decision tree becomes: (assuming this is a mutator thread, and GC is already initialized)
The control flow is more linear than before, with three steps, each using one boolean option. By combining the three options, we can replicate the behaviors of the previous
We can make a new combination to poll (scheduling GC in the background) and overcommit at the same time, and postpone blocking for GC to the next safepoint. This can be useful for VMs where allocation never happens at safepoints.
But I wonder whether we can remove the |
We discussed in today's meeting. We should either
I am in favor for removing The only thing it may affect may be NoGC. NoGC panics immediately in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than #1400 (comment), this PR looks good to me.
@@ -1,6 +1,8 @@ | |||
// GITHUB-CI: MMTK_PLAN=NoGC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this test only for NoGC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it doesn't have to. The test is about the behavior of allocation after exceeding the heap size, and it is not about the GC. So it doesn't really matter which plan it is. But I changed it to "all" just in case any plan triggers GC differently (mainly ConcurrentImmix).
Now polling cannot be disabled. It will always poll.
I removed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR looks good to me.
There are just some minor points about the documentation -- I think it currently over-specifies the behavior of the new boolean flags. It’s good to be specific and precise, but the docs shouldn’t expose internal implementation details or define behaviors that are not controlled by the flag.
src/util/alloc/allocator.rs
Outdated
/// Whether over-committing is allowed at this allocation site. | ||
/// | ||
/// **The default is `false`**. | ||
/// | ||
/// If `true`, the allocation will still try to acquire pages from page resources even when a GC | ||
/// is triggered by the polling. | ||
/// | ||
/// If `false` the allocation will not try to get pages from page resource as long as GC is | ||
/// triggered. | ||
/// | ||
/// Note that MMTk lets the GC trigger poll before trying to acquire pages from the page | ||
/// resource. This gives the GC trigger a chance to trigger GC if needed. `allow_overcommit` | ||
/// does not disable polling, but only controls whether to try acquiring pages when GC is | ||
/// triggered. | ||
pub allow_overcommit: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the doc is way more detailed than what is necessary.
None of these is related with 'over commit':
- MMTk will acquire pages
- MMTk uses page resources
'Overcommit' only means one thing: MMTk may go beyond the specified heap size, in order to satisfy this allocation request. Additionally, MMTk still triggers GC when it overcommits memory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I'll simplify the description and make the point that "when over-committing, it may allocate beyond the heap size".
/// Whether the allocation is at a safepoint. | ||
/// | ||
/// **The default is `true`**. | ||
/// | ||
/// If `true`, the allocation is allowed to block for GC, and call [`Collection::out_of_memory`] | ||
/// when out of memory. Specifically, it may block for GC if any of the following happens: | ||
/// | ||
/// - The GC trigger polled and triggered a GC before the allocation tries to get more pages | ||
/// from the page resource, and the allocation does not allow over-committing. | ||
/// - The allocation tried to get more pages from the page resource, but failed. In this | ||
/// case, it will force a GC. | ||
/// | ||
/// If `false`, the allocation will immediately return a null address if the allocation cannot | ||
/// be satisfied without a GC. It will never block for GC, never force a GC, and never call | ||
/// [`Collection::out_of_memory`]. Note that the VM can always force a GC by calling | ||
/// [`crate::MMTK::handle_user_collection_request`] with the argument `force` being `true`. | ||
pub at_safepoint: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. at_safepoint
means MMTk may block the thread in this allocation request for GC. These are not related with at_safepoint
:
- MMTk may call
out_of_memory
. MMTk callsout_of_memory
when it runs out of memory, which is unrelated withat_safepoint
. Before we have a specific flag likeallow_oom_call
, there is no definition when MMTk may callout_of_memory
-- it is an implementation detail. - The reasons why MMTk may block for GC are implementation details.
handle_user_collection_request
is unrelated. It is a separate API, and is not related withalloc_with_options
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still have a method allow_oom_call
. It previously returns true
only for OnAllocationFail::RequestGC
. Maybe we should add another option AllocationOptions::allow_oom_call
for that.
handle_user_collection_request
is relevant because if alloc_with_option(at_safepoint=false)
cannot force a GC, and it can't satisfy the allocation request without a GC, the user would trigger a GC manually instead. Currently, it mimics the behavior of OnAllocationFail::RequestGC
and it forces a GC when it is at safepoint, and GC is initialized.
Alternatively, we can make "forcing GC" (i.e. the second poll()
invocation with space_full = true
) unstoppable, too. That is, as long as we fail to get pages from the page resource, it will force a GC. But it may return null
if it is not at a safepoint. This will slightly change the control flow. One concern is that what will happen if GC is not initialized, and we failed to get pages from the PR, and it is not at safepoint? Should it panic immediately, or should it return null? @qinsoon what do you think of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My opinions on this are:
- Binding users do not need to know the 'forced GC'. There is little point mentioning it.
at_safepoint
only makes sure the thread will not be blocked in this call for GC. It does not do anything more than that.- Whether we 'force' GC after a failed allocation is an implementation detail.
- I personally think we probably still want to 'force' the GC after a failed allocation, but do not block the current thread. In this case,
at_safepoint
only changes the behavior of blocking, and does not change the behavior of GC triggering.
One concern is that what will happen if GC is not initialized, and we failed to get pages from the PR, and it is not at safepoint? Should it panic immediately, or should it return null?
With at_safepoint=true
, the behavior is panic. We could keep that behavior with at_safepoint=false
. at_safepoint
doesn't need to change that behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the documentation so that at_safepoint
no longer guarantees anything other than not blocking for GC.
I also made allow_oom_call
another option. But the current behavior of allow_oom_call
is quite inconsistent. It only controls Space::handle_obvious_oom_request
and LockFreeImmortalSpace::acquire
, but not Allocator::alloc_slow_inline
or util::memory::handle_oom_error
both of which call Collection::out_of_memory
. I didn't change the current behavior, and I am leaving it to another pull request.
It should be "at_safepoint" instead of "allow_oom_call".
We remove
OnAllocationFail
and add boolean fields toAllocationOptions
:allow_overcommit
: whether we allow overcommitat_safepoint
: whether this allocation is at a safepointallow_oom_call
: whether to callCollection::out_of_memory
Now
Space::acquire
always polls before trying to get new pages. Particularly, whenallow_overcommit == true
, polling and over-committing will happen in one allocation attempt. If we also setat_safepoint == false
, the current mutator will be able to allocate normally in this allocation, but block for GC at the nearest safepoint. This is useful for certain VMs.